Add method to convert set options dictionary to string format for Docker Buildx Bake#361
Add method to convert set options dictionary to string format for Docker Buildx Bake#361ianpittwood wants to merge 2 commits intomainfrom
Conversation
Test Results1 113 tests 1 113 ✅ 12m 56s ⏱️ Results for commit 604d282. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an error that occurred when using --strategy bake with --temp-registry by adding a method to convert set options from Python dictionary format to the comma-delimited string format expected by Docker Buildx Bake's --set parameter.
Changes:
- Added
_set_opts_dict_to_strstatic method to convert dictionary set options to string format for Docker Buildx Bake - Changed the
*.outputset option from a list containing a dictionary to just a dictionary, which is then properly converted to string format
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| posit-bakery/posit_bakery/image/bake/bake.py | Added _set_opts_dict_to_str method to convert set options to string format and integrated it into the build method |
| posit-bakery/posit_bakery/config/config.py | Changed *.output from list format to dict format to align with the conversion method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for opt, data in set_opts.items(): | ||
| if isinstance(data, list): | ||
| set_opts[opt] = ",".join(data) | ||
| elif isinstance(data, dict): | ||
| set_opts[opt] = ",".join(f"{k}={v}" for k, v in data.items()) | ||
| else: | ||
| set_opts[opt] = str(data) | ||
|
|
||
| return set_opts |
There was a problem hiding this comment.
The method mutates the input dictionary directly by modifying set_opts in place. This can lead to unexpected side effects if the caller retains a reference to the original dictionary. Consider creating a new dictionary instead of modifying the input. For example, you could initialize a new dictionary result = {} and populate it in the loop, then return result instead of set_opts.
| for opt, data in set_opts.items(): | |
| if isinstance(data, list): | |
| set_opts[opt] = ",".join(data) | |
| elif isinstance(data, dict): | |
| set_opts[opt] = ",".join(f"{k}={v}" for k, v in data.items()) | |
| else: | |
| set_opts[opt] = str(data) | |
| return set_opts | |
| result: dict[str, str] = {} | |
| for opt, data in set_opts.items(): | |
| if isinstance(data, list): | |
| result[opt] = ",".join(data) | |
| elif isinstance(data, dict): | |
| result[opt] = ",".join(f"{k}={v}" for k, v in data.items()) | |
| else: | |
| result[opt] = str(data) | |
| return result |
| set_opts[opt] = ",".join(data) | ||
| elif isinstance(data, dict): | ||
| set_opts[opt] = ",".join(f"{k}={v}" for k, v in data.items()) | ||
| else: | ||
| set_opts[opt] = str(data) |
There was a problem hiding this comment.
When converting values to strings, Python boolean values will be rendered as "True" or "False" (capitalized). However, Docker Buildx expects lowercase boolean values ("true", "false") in set options. This affects both the dictionary values in line 203 and the else branch in line 205. Consider adding special handling for boolean values: str(v).lower() if isinstance(v, bool) else str(v) to ensure proper formatting for Docker.
| set_opts[opt] = ",".join(data) | |
| elif isinstance(data, dict): | |
| set_opts[opt] = ",".join(f"{k}={v}" for k, v in data.items()) | |
| else: | |
| set_opts[opt] = str(data) | |
| set_opts[opt] = ",".join( | |
| str(v).lower() if isinstance(v, bool) else str(v) for v in data | |
| ) | |
| elif isinstance(data, dict): | |
| set_opts[opt] = ",".join( | |
| f"{k}={str(v).lower() if isinstance(v, bool) else str(v)}" | |
| for k, v in data.items() | |
| ) | |
| else: | |
| set_opts[opt] = str(data).lower() if isinstance(data, bool) else str(data) |
| @staticmethod | ||
| def _set_opts_dict_to_str(set_opts: dict[str, Any]) -> dict[str, str]: | ||
| """Convert a dictionary of set options to a comma-delimited, key=value string format for Docker Buildx Bake. | ||
|
|
||
| :param set_opts: A dictionary of set options to convert. | ||
|
|
||
| :return: A dictionary of set options with string values. | ||
| """ | ||
| for opt, data in set_opts.items(): | ||
| if isinstance(data, list): | ||
| set_opts[opt] = ",".join(data) | ||
| elif isinstance(data, dict): | ||
| set_opts[opt] = ",".join(f"{k}={v}" for k, v in data.items()) | ||
| else: | ||
| set_opts[opt] = str(data) | ||
|
|
||
| return set_opts |
There was a problem hiding this comment.
The new _set_opts_dict_to_str method lacks test coverage. Given that the repository has comprehensive test coverage for similar methods (as seen in test_bake.py with tests for other BakePlan methods), consider adding tests to verify the correct conversion of different data types (lists, dicts, strings, numbers, booleans) to their string representations. This is especially important since this method handles the critical task of formatting parameters for Docker Buildx Bake.
| """ | ||
| for opt, data in set_opts.items(): | ||
| if isinstance(data, list): | ||
| set_opts[opt] = ",".join(data) |
There was a problem hiding this comment.
The join operation assumes all list items are strings. If the list contains non-string types (e.g., numbers, booleans), this will raise a TypeError. Consider converting list items to strings: ",".join(str(item) for item in data). Note that boolean values should be lowercased for Docker compatibility, so you may need ",".join(str(item).lower() if isinstance(item, bool) else str(item) for item in data).
| set_opts[opt] = ",".join(data) | |
| set_opts[opt] = ",".join( | |
| str(item).lower() if isinstance(item, bool) else str(item) | |
| for item in data | |
| ) |
Fixes the following error message when using
--strategy bakewith--temp-registryset: